@CodeRabbit review this pr

#!/bin/bash
# Get PR information using GitHub CLI
gh pr view 7740 --repo ecamp/ecamp3 --json title,body,files,additions,deletions,changedFiles,baseRefName,headRefName,author,state,mergeable

# Get the diff for the PR
gh pr diff 7740 --repo ecamp/ecamp3

@BacLuc Great work on implementing this security fix! This PR effectively prevents privilege escalation by guests. Here's my comprehensive review:

✅ Security Implementation

The core security logic is solid:

• Guests cannot elevate their roles to member/manager (blocks privilege escalation)
• Members/managers can still downgrade their roles (preserves legitimate functionality)
• Only applies to users modifying their own collaborations (other permissions handled by voter)

🔧 Code Quality Issues

1. Unused Dependency: CampRoleVoter is injected but never used in the code:

private readonly CampRoleVoter $campRoleVoter, // ← Remove this

2. Unclear Error Message: The generic "Not authorized to change role" could be more specific:

throw new HttpException(403, 'Guests cannot elevate their role to member or manager');

3. Code Clarity: The logic could be more readable:

public function onBeforeRoleChange(CampCollaboration $data, CampCollaboration $previous): CampCollaboration {
    $currentUser = $this->security->getUser();
    $isOwnCollaboration = $data->user->getId() === $currentUser->getId();
    
    // Only check role escalation for own collaborations
    if (!$isOwnCollaboration) {
        return $data;
    }
    
    $previousRole = $previous->role;
    $canElevateRole = in_array($previousRole, [CampCollaboration::ROLE_MANAGER, CampCollaboration::ROLE_MEMBER], true);
    
    if (!$canElevateRole) {
        throw new HttpException(403, 'Guests cannot elevate their role to member or manager');
    }
    
    return $data;
}

📋 Testing Coverage

Excellent test coverage! The tests cover:

• ✅ Guest role escalation prevention
• ✅ Member/manager role changes
• ✅ Status changes
• ✅ Combined status/role changes

Minor improvement - some test method names could be more descriptive:

// Instead of:
testPatchOwnCampCollaborationToMemberIsRejectedForGuest()
// Consider:
testGuestCannotElevateOwnRoleToMember()

🏗️ Architecture Notes

The PropertyChangeListener pattern is well-implemented. The changes to support 1-2 parameters in PropertyChangeListener.php are necessary and correctly implemented.

Your note about potentially moving this to CampRoleVoter later is valid - this would centralize authorization logic, but the current approach works well for the immediate need.

🚀 Overall Assessment

This is a solid security improvement with good test coverage. The main issues are minor code quality improvements (unused dependency, clearer error messages) rather than functional problems.

Recommendation: ✅ Approve after addressing the unused dependency.

🐰

   (\   /)
  ( ._. )
 o_(")(")

@CodeRabbit i fixed some issues

#!/bin/bash
# Get the latest PR diff to see current changes
gh pr diff 7740 --repo ecamp/ecamp3

# Get current PR status
gh pr view 7740 --repo ecamp/ecamp3 --json title,body,files,additions,deletions,changedFiles,commits,updatedAt

@BacLuc Excellent work on the updates! I can see you've addressed several key issues from my previous review:

✅ Fixed Issues

1. Unused Dependency Removed: The CampRoleVoter dependency has been properly removed from the constructor - great catch!

2. Proper Imports Added: Added the necessary User and HttpException imports that were missing.

3. Significantly Expanded Test Coverage: Outstanding improvement! The test suite now covers:

   • Guest role escalation prevention (both to member and manager)
   • Member role changes (including elevation to manager)
   • Manager role changes (including demotion)
   • Status changes for different roles
   • Combined status and role changes
   • Much more comprehensive edge cases

4. Test Code Improvements: Fixed the test method calls to use getFixture() instead of static::$fixtures for better consistency.

5. PropertyChangeListener Enhancement: Properly implemented support for 1-2 parameters as needed.

💡 Minor Remaining Opportunities

The security implementation is solid and functional! A couple of minor refinements could still be considered:

• Error Message: Still shows the generic "Not authorized to change role" - a more specific message like "Guests cannot elevate their role to member or manager" would be clearer for users.

• Code Readability: The logic works perfectly but could be slightly more readable with variable names that make the intent clearer.

🎯 Overall Assessment

This is a significant improvement! The expanded test coverage is particularly impressive and gives great confidence in the security fix. The main functional issues have been resolved.

Status: ✅ Ready for merge - the security implementation is solid and well-tested.

Great work on addressing the feedback so thoroughly! 🚀

It's okay for me to merge now, to fix this one case. However I'd like to have a more general fix in the voter some time. The voter could load the original data of $subject before the change from the db and perform the permission check on that old version of the object. Or, to gain access to the request context, like AssertAllowTransitionsValidator, the Voter could get a RequestStack object and then get the old version of the subject via $this->requestStack->getCurrentRequest()->attributes->get('previous_data') (but maybe make sure the subject is really equivalent to the entity being mutated, by checking iri equality)

• Use previous state of CampCollaboration for ACL check in CampRoleVoter #8125